Repository navigation
Conversation
devsuitup
left a comment
There was a problem hiding this comment.
Review at 401e27e. Thanks for the PR. The idea is right: after /clear the new transcript should belong to the running session. The problems are in how the new transcript is attributed and how the session is re-keyed. The reviewer traced the paths below in the source. I re-read the code behind the first two; I have not run them.
Blocking
- An unrelated
/clearcan be attached to our terminal (session-transitions.js, thesignals.clearedbranch).matchedis true whenowner === 'unknown' && isSoleClaudeIn(folder). On Windows and macOS the ownership of a transcript cannot be established, soownerisunknown. If one managed session runs in a folder and the user runsclaudeoutside the app in the same folder and clears it, that other session's new transcript is taken over by our terminal. - Re-keying removes the id that in-flight triggers use (same block:
activeSessions.delete(sessionId)thenset(newId, ...))./clearis one of the two commands the trigger watcher may send, and a chain resolves its target by session id at each step. After the/clearstep the old id no longer exists, so the following steps abort even though the terminal is alive. - A pending owner has no retry (same block). A file in the
pendingstate is only rechecked when something else triggers a scan, and a partially written header can stay excluded from detection for good. - Quit after
/clear, before the first prompt (public/app.js). The saved new id is not in the index yet, so restore cannot find it.
Non-blocking
test/session-transitions.test.js(the awaiting-fork case) passes even though an older snapshot matcher causes a transition, so it does not pin what it says.CHANGELOG.md: the entry needs the PR number as its suffix,(#477).- #482's archive state and the injected pending row may disagree.
The branch is also two commits behind main, with three overlapping files.
Six existing suites pass against the unchanged sources (81 tests, Node 24). Not verified: a real restart with SQLite, Node 20/22 with c8.
|
Tested on Linux (Ubuntu, GNOME on Wayland), PR head 401e27e. Setup. An isolated instance from the PR head, with a temp HOME, its own
Blocking 1 (an unrelated
Blocking 2 (a trigger chain with
Blocking 3 (pending owner with no retry): not observed. In every live Blocking 4 (quit after
|
|
Blocking 1 of my review (an unrelated Driven in memory with the shipped
So on Windows (and macOS, which shares the null reader), the single-managed-session fallback accepts any external Not run: a real CLI or Electron, Node 20/22 with c8. |
… it apart /clear makes the CLI open a new jsonl under a new session id that names nothing of the one it replaced, so fork detection never matched it: the terminal stayed on the old row and the new conversation appeared as a separate, unattached session. The CLI's state file switches to the new id on /clear; resolve its pid up to the PTY's and re-key the session like a fork.
…ins on it An owner that cannot be established (no /proc: macOS, Windows) was matched when one Claude PTY ran in the folder, so a claude run outside the app could be taken over; it is now never matched. A re-key kept no trace of the old id, so a trigger chain that sent /clear lost its terminal: the trigger context now resolves ids through the re-keys. A pending owner, or a new file whose first records are not written yet, is rechecked a second later while fresh, instead of waiting for another change in the folder. The snapshot-only fork matcher no longer takes a /clear file. A session with no transcript yet is left out of the saved set, which is saved again once its first prompt makes it real.
|
Thanks to both of you, and thanks @devsuitup for the Windows reproduction of point 1. Rebased on current main (eaf59ff); the conflicts with #374/#487 in Blocking
Non-blocking
Other changes
Each fix was removed in turn, and each removal fails a test: the unknown fallback, the alias record, the PTY and status resolution, the recheck, the partial-file recheck and its age limit, the snapshot exclusion, the persist exclusion and the persist on becoming real. On the full suite, the only failures are the CLI-state canary, |
devsuitup
left a comment
There was a problem hiding this comment.
Review at 4a670ad, rebased on current main. Thanks: the unknown-owner fallback no longer attaches a stranger's transcript, the missing-id lookup and the awaiting-fork assertion are fixed, the pending-owner retry works, the changelog suffix is there and the docs are updated. 176 tests in 12 related suites pass locally on Node 24, and the reviewer reproduced the findings below with the shipped modules. Four points block; three of them were already reproduced on Linux in the maintainer's test of 401e27e.
Blocking
- A header-only transcript is marked known before
/clearappears, so it never transitions (session-transitions.js~495, the finalknownJsonlFilesupdate). A flush that reads only the initial mode/caveat records seescleared === false; the file is not put inemptyFiles, so it joinsknownJsonlFilesand the/clearrecord appended later never reconsiders it. Keep a new file with a header and no command record eligible (as the pending-owner case does) until it shows a first user turn or goes stale. Test: write the header, flush, append the/clearrecord, flush again, and expect the transition. - Quitting after
/clearand before the first prompt loses the tab on restore (public/app.js~185).persistWorkingSetsaves the new id; the new transcript is bookkeeping-only, so it is never indexed and the restore planner reports "not in the index". Reproduced on Linux: "Not restored: is not in the index". Persist enough to restore the cleared session independently of indexability (or keep the old id as the restorable target until a prompt exists). Test: clear, persist, restart the planner with that set, and expect the session restored. - An awaiting fork accepts another session's snapshot prefix (
session-transitions.js~450). A session waiting for its fork matches any new transcript that has only snapshots and no/clearmarker, before any ownership evidence arrives. Require theforkFromor ownership evidence before matching, or wait for the marker. - The old and the new id take different trigger locks (
trigger-context.js~44,trigger-watcher.js~1843).sessionLocksis keyed by the raw session id, but both ids reach the same PTY after a re-key, so triggers aimed at the old and the new id can interleave their writes. Key the lock by the PTY (or resolve the id throughrekeyedbefore locking). The chain that aborts at step 0 on Linux ("session exited during chain step 0 submit verify") needs the in-flight target followed through the transition as well; please cover it with a test that runs[/clear, reply]through the shipped watcher.
Non-blocking
- Pending-row injection in the renderer bypasses the backend archive filtering (
public/app.js~1171): an archived cleared session can reappear. - An original trigger id stops resolving after 33 clears (
session-transitions.js~34, the bounded map). - Explanatory comments at
session-transitions.js~31 and ~449 go beyond the one-line pointer allowed in code. test/clear-pending-persist.test.js(~43) supplies the pending rows by hand, so the new renderer callback branch is not pinned.
Not run: a live /clear chain, a SQLite restart, macOS, Node 20/22 with c8, lint, fresh CI. CI on this head is not approved yet.
Problem
/clearmakes the CLI open a new jsonl under a new session id. Nothing in it names the session it replaced (noforkedFrom; every record carries the new id), sodetectSessionTransitions()never matched it: the open terminal stayed on the old row and the new conversation appeared as a separate, unattached sidebar entry once its first prompt made it indexable.Fix
The CLI rewrites
~/.claude/sessions/<pid>.jsonwith the newsessionIdon/clear(verified on live state files).cliSessionState.clearOwner(newId, ptyPid)finds the live pid namingnewIdand walks its/procparent chain up to the PTY:mine→ re-key exactly like a fork (session-forked).other→ another process's file; ignored.pending→ state file not rewritten yet; rechecked for up to 60 s.unknown(no/proc: macOS/Windows) → matched only when this is the sole live Claude PTY in the folder.readNewSessionSignals()recognises the file by its first non-bookkeeping user record (classifyUserText). The renderer adds a pending "New session" row for the new id, since a/cleartranscript is not indexed until its first prompt. The cleared conversation stays in the list as a past, resumable session.Docs:
.ai/contexts/cli-session-state.md→ "Owner of a /clear transcript".Tests
test/session-transitions.test.js(mine / other / pending→mine / stale pending / unknown sole vs. two candidates / prompt-not-clear / post-fork clear / awaiting fork), 1 intest/cli-session-state.test.js(clearOwnerverdicts).🤖 Generated with Claude Code